Repository navigation
Conversation
|
build_jenkins |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new matcher has unsupported ordering and dynamic-shape failure cases without regression coverage.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Extends GPU compressed fully connected conversion to accept runtime scale and zero-point tensors.
Changes:
- Expands decompression patterns to Parameter inputs.
- Adds runtime reshape/conversion handling.
- Preserves oneDNN decompression input formats.
Risks: Dynamic shapes can throw, one scale ordering is unmatched, and regression tests are missing.
| File | Description |
|---|---|
convert_fc_to_compressed.cpp |
Handles non-constant decompression inputs. |
compressed_weights_pattern.hpp |
Matches Parameter-based scale/ZP paths. |
fully_connected_onednn.hpp |
Preserves scale/ZP input formats. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
### Details: The reason for this refactor is to make the code cleaner and better prepare for upcoming features: - Support for [num_group, N, group_size] weights layout (openvinotoolkit#38206) - Support for scale/zp as parameters (openvinotoolkit#38204) #### Changes: 1. Use the new pattern matching operators (`| and optional`) to simplify the patterns. 2. Add `transpose_b_scale` and `transpose_b_zp` to track the actual layout of scale/zp. **Current logic:** 1. If weights and scale/zp are transposed, scale/zp will be reordered back to a non-transposed layout to satisfy kernel requirements. 2. If weights and scale/zp are not transposed, weights and scale/zp will first be reordered to a transposed layout. Since the kernel still requires scale/zp in a non-transposed layout, another reorder is added to convert scale/zp back. As a result, in case 2, scale/zp are unnecessarily transposed and then transposed back. This increases memory usage and adds extra compilation work. The impact is currently limited because the tensors are constants and the cost is paid only during compilation. **With this PR**: By tracking the actual layout of scale/zp, the plugin can skip the unnecessary transpose-and-revert steps in case 2. This simplifies the logic, reduces memory usage, and will be especially helpful when weights/scale/zp are provided as parameters instead of constants. ``` Data flow (non-transposed case) Weights [K,N] --Transpose--> FC (transpose_b=true) Scale [G,N] -------------> FC (transpose_b_scale=false) // no Transpose, no reorder ZP [G,N] -------------> FC (transpose_b_zp=false) OCL : bfyx[G,N] reinterpreted as fbyx (N=batch, G=feature) oneDNN: ngroups read from axis 0 ([G,N]) ``` ### Tickets: - *[CVS-195001](https://jira.devtools.intel.com/browse/CVS-195001)* ### AI Assistance: AI assistance used to implement fix and unit test. The generated code was verified for correctness, manually built, executed, and fixed as needed. --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Take v9's fully_connected_onednn.cpp (explicit transpose_b_scale/zp flags supersede the PR's shape heuristic), fix the PR's unbalanced query_formats block, keep v9's GNK, Slice zero-point and transpose_before_reshape handling, and let the GNK branch take scale/zp Parameters.
05d1e44 to
d9e5759
Compare
|
Add support for Conv1x1 ->FC Compressed and update the PR with refactored implementation based on transpose_b_scale and transpose_b_zp flags introduced in #38697 |
|
The PR is based on older master, which likely contributed to some unintentional changes. Will update it against latest master and remove unrelated code. |
a2a07c0 to
167732b
Compare
|
@isanghao Updated PR and description broadly based on previous review. Please check |
cbc60ce to
23e6c25
Compare
b666cfa to
fe4ee23
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Runtime-shaped scale/zero-point Reshapes can trigger a static-shape assertion, while the Parameter-scale Multiply path omits required runtime metadata and fails the runtime-info check.
3 open issues
8 resolved since last review
Vector scale leaves weight and input ranks mismatched Avoid division by zero for zero-length reduction dimensions Skip dynamic quantization for Hadamard-fused u2 FCs Keep asymmetric u2 activations on the f16 path Match converted scale forms when a zero point is present Propagate runtime info to the post-reshape Multiply Add transformation coverage for parameter-backed reshape branches Add u2 × i8 execution and threshold-guard coverage
Review effort: Balanced
8090c2a to
7633546
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Blocked scale and zero-point buffers can reach oneDNN’s contiguous descriptors without reordering, causing padding to be read as values and producing incorrect results.
4 open issues
3 resolved since last review
Review effort: Balanced
08cd784 to
c8d04f3
Compare
e-ddykim
left a comment
There was a problem hiding this comment.
@Kotomi-Du Please check the below issues.
- f32 scale
Parameterloses itsConvert(crash).mul_const_convert_mabsorbsConvert(f32->f16), but the callback readspattern_map.at(mul_const_m), so a runtime scale reaches the FC as f32 -->NodeValidationFailure(make_try_fold<Multiply>(f32, f16)) when the post-reshapemul2tail exists. The parameterized test currently pins this stripped behavior. Reshape(Convert(zp_u8, f16))passes an f16 zero point toFullyConnectedCompressed. oneDNN'svalidate_implonly allows{i4,u4,u8,i8}--> silent fallback off XMX, or hard "Failed to select implementation" with non-transposed sub-byte parameter weights;DynamicQuantizeFullyConnectedrejects it too. The weights path already drops its Convert and reshapes the integer source — ZP should do the same.- Bare
Convert(scale_param)is unmatched when a ZP is present (mul_value_param_convert_mdefined but not inmul_const_m). This is exactly whatConvertPrecisioninserts for 2D f32 scale parameters, so those FCs silently stay uncompressed. [1, N, K]weights +[1, N, 1]scale/zp crash.MoveFCReshapeToWeightsmatches Constants only, so with Parameter forms the rank-3 scale is misread as a group axis and collapses the wrong dims ([1,32,16] -> [1,512]vs K=16).- Per-tensor (1-element) runtime scale/zp reads OOB in OCL. Unlike Constants, a
[1,1]Parameter ZP is never folded toDECOMPRESSION_ZP_SCALAR, andfully_connected_gpu_gemv.clindexes it per output channel. TheSINGLE_GROUP_NUMbranches (K > 128, i.e. all real LLM shapes) and and theOUTER_OFM > 1sites inbf_tiled.cl(+common) need the sameLENGTH == 1 ? 0 : ...guard.
I pushed a single commit with fixes + regression tests on top of your head. Please check this commit --> e-ddykim@6090f7e



Summary:
As a follow up of #34767 which is to recognize parameters as valid inputs for weights, this PR extends the same support to scale and zero-point (ZP) tensors. The scope doesn't cover conv1x1, because OVEP has blocked scale/zp promotion for conv operator in this related PR
Details:
Recognize scale / zero-point as model inputs in
ConvertFullyConnectedToFullyConnectedCompressed(Intel GPU plugin). Previously the decompression scale and zero point had to beConstants; now they can also come from aParameter.compressed_weights_pattern.hpp: scale (Multiply) and zero-point (Subtract) operands now match:ConstantParameterReshape(Parameter)Reshape(Convert(Parameter))convert_fc_to_compressed.cpp:reshape_constbecamereshape_input. Constants are still folded into a newConstant; any other input gets aReshapenode.convert_zp_constdecides on aConvertfrom the input's element type, so it works for constant and non-constant zero points. Au8zero point is passed through unchanged.oneDNN FC keeps the scale / zero-point input format (
fully_connected_onednn.hpp).query_formatsnow keeps the existing format of the decompression scale and zero-point inputs instead of forcing the default format.fbyxfromprepare_quantizationfor grouped transposed scale/zp. Without it, oneDNN would force a reorder back to the default format.Tests
convert_fc_to_compressed_test.cpp: addedConvertFCToCompressedParameterScaleZpTest.Reshape(Convert(Parameter))orConvert(Reshape(Parameter)), with and without a zero point.FullyConnectedCompressedthat takes the parameter scale/zp directly.convert_matmul_to_fc_test.cpp: runsConvertMatMulToFullyConnectedfollowed byConvertFullyConnectedToFullyConnectedCompressed.FullyConnectedCompressed.Parameter([?, 1]) leaves the MatMul unchanged. These are regression tests for theshape_b.is_static()assert.fully_connected_gpu_test.cpp: new general (not oneDNN-only) testtest_compressed_int8_parameter_scale_zp.Setup: u8 weights
[N, K], scale/zp asinput_layoutwithbfyxdata.Accuracy: output is compared with a reference computed as
(w - zp) * scale.Layout checks per case:
compressed_int8_parameter_scale_zp_per_channel[1, N]bfyxcompressed_int8_parameter_scale_zp_per_channel_transposed[N, 1]bfyx(already N-innermost, no reorder)compressed_int8_parameter_scale_zp_per_group[G, N]bfyxcompressed_int8_parameter_scale_zp_per_group_transposed[N, G]reorderbfyx→fbyxinserted byprepare_quantizationTickets:
AI Assistance:
RelWithDebInfo, Windows):ov_gpu_unit_tests --gtest_filter=TransformationTestsF.ConvertFCToCompressed*:*FullyConnected*Compressed*: 27 passed.ov_gpu_unit_tests --device_suffix=0 --gtest_filter=fully_connected_gpu_tests.compressed*(iGPU, UHD 770): 88 passed.ov_gpu_unit_tests --device_suffix=1 --gtest_filter=fully_connected_gpu_tests.compressed*(dGPU, with hardware matmul): 41 passed, 47 skipped by existing device-capability checks, 0 failed.compressed_int8_parameter_scale_zp*tests passed on both iGPU and dGPU